Volume geometry type #417
Volume geometry type #417
Conversation
(waiting until it's actually ready causes problems)
(cherry picked from commit f1af28e)
frasercl
requested review from
interim17 and
ShrimpCryptid
and removed request for
a team
|
Looks like the unit tests/type checks are failing... Could you fix those? edit: ah, see the comment in the description now. Will review anyways |
| if (this.image) { | ||
| this.image.setTranslation(new Vector3(data.x, data.y, data.z)); | ||
| this.image.setRotation(new Euler(data.xrot, data.yrot, data.zrot)); | ||
| const r = data.cr * 2; |
There was a problem hiding this comment.
What does cr stand for? radius?
There was a problem hiding this comment.
The r is definitely radius and the c I'm almost certain is "collision."
| private async fetchVolume(url: string): Promise<VolumeModel> { | ||
| // TODO should this be in a worker? Are we already in a worker here? | ||
| // Should this class get a `VolumeLoaderContext` going? | ||
| const model = new VolumeModel(); | ||
| this.setGeometryInRegistry(url, model, GeometryDisplayType.VOLUME); | ||
| const loader = await createVolumeLoader(url); | ||
| // TODO onChannelLoaded callback? | ||
| const volume = await loader.createVolume(new LoadSpec()); | ||
| model.setImage(volume); | ||
| return model; | ||
| } | ||
|
|
There was a problem hiding this comment.
Not sure if this relevant, but the caching changes in review are removing the need for workers in that part of the code base, and there was discussion of workers affecting builds and publishing with NBSV.
Just a thought that we should use them if we need them, but if achieve 0 workers (automation fears??) then there may be some arguments for not adding them back in?
There was a problem hiding this comment.
unless I am forgetting, doesn't PDB geometry still use a worker to calculate levels of detail?
There was a problem hiding this comment.
The context here may be slightly different. In volume-viewer we moved loading to a separate worker because playing volume time series requires loading and processing quite a lot of data per frame, and it was a problem when that processing blocked rendering. We have an externally available type, called VolumeLoaderContext, for handling offloading that processing to a worker. Questions are
- is it as worth it to use
VolumeLoaderContexthere as it is in volume-viewer, given your concern and that simulation volumes may be smaller on average than microscope volumes; and - if 1, who should own
VolumeLoaderContextand how should it grant access to it?
To be resolved by future work.
Review time: smallish (10-20min)
Resolves #406 (and #405?): adds a volume agent and geometry type using imports from volume-viewer, with enough rendering code to show the volume's bounding box.
NOTE: per the strategy discussed in #405, this change pulls in volume-viewer as a file dependency. The code in this PR only works if volume-viewer is built from the
simularium-volumesbranch in a sibling directory to this repo. (Also, we may have to loosen some checks to let this and future PRs on this feature merge?)Screenshot
you can tell it's at least loading volume metadata because the box is the right shape